Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[core] Add authentication to Toolpad Core #3609

Merged
merged 140 commits into from
Aug 2, 2024
Merged

[core] Add authentication to Toolpad Core #3609

merged 140 commits into from
Aug 2, 2024

Conversation

bharatkashyap
Copy link
Member

@bharatkashyap bharatkashyap commented May 27, 2024 •
edited
Loading

Closes #3419

ToolpadCoreAuth.mov

Sign In Page docs
Account docs

To Do List

  • Demos and docs for SignInPage component
  • Demos and docs for User Account component
  • Add jsdoc to properties for both components to help with vscode intellisense
  • Allow customising components through a slotProps API
  • Test auth examples functioning with packages from this PR
    • core-auth-nextjs working
    • core-auth-nextjs-pages working

Follow-up

  • Mention which OAuth Providers are supported
  • <Account /> demo spacing/layout shift issues
  • Finish the credentials provider:
    • RegisterPage component
    • ChangePasswordPage component
  • Allow customisation through a slots API
  • Support multiple layout versions of the SignInPage
  • Add email/magic links provider and build an example: https://next-auth.js.org/providers/email

Open topics

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 27, 2024
@bharatkashyap bharatkashyap changed the title [feat] Add authentication components to Toolpad Core [core] Add authentication components to Toolpad Core May 29, 2024
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jun 10, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 13, 2024
@bharatkashyap bharatkashyap changed the title [core] Add authentication components to Toolpad Core [core] Add authentication to Toolpad Core playground app Jun 13, 2024
@bharatkashyap bharatkashyap marked this pull request as ready for review June 13, 2024 11:52
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 16, 2024
@Janpot Janpot added the core Infrastructure work going on behind the scenes label Jun 18, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 18, 2024
@Janpot
Copy link
Member

Janpot commented Jun 18, 2024 •
edited
Loading

Shaping a bit more the SIgnIn page. For now let's concentrate purely on this component

  • clean up implementation
  • keep ‘auth’ in the page paths
  • start building a pages router version (untested)
  • extract SignInPage client component
  • integrate branding provider
  • make framework agnostic (e.g. remove next.js Image component)
  • rename env vars, don’t call them TOOLPAD_…
  • start on adding credentials provider support
  • fix some spacing issues
  • ...

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 28, 2024
@Janpot Janpot requested a review from apedroferreira July 29, 2024 13:49
apedroferreira
apedroferreira approved these changes Jul 29, 2024
View reviewed changes
Copy link
Member

@apedroferreira apedroferreira left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went mostly through the docs, everything seems good so nothing I wrote should be a major blocking change, I think. Let me know if there are any specific things besides docs that you'd like me to take a look at.

Also one more thing: the forgot password links in the demos are kind of weird as they link to the main MUI home page, can we do something about that?

docs/data/toolpad/core/components/sign-in-page/sign-in-page.md Show resolved Hide resolved
docs/data/toolpad/core/components/sign-in-page/BrandingSignInPage.js Show resolved Hide resolved
docs/data/toolpad/core/components/sign-in-page/sign-in-page.md Outdated Show resolved Hide resolved
docs/data/toolpad/core/components/account/account.md Outdated Show resolved Hide resolved
Janpot
Janpot reviewed Jul 30, 2024
View reviewed changes
docs/data/toolpad/core/components/sign-in-page/sign-in-page.md

## OAuth

The `SignInPage` component can be set up with an OAuth provider by passing in a list of providers in the `providers` prop, along with a `signIn` function that accepts the `provider` as a parameter.
Copy link
Member

@Janpot Janpot Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be done in follow-up

We should also explain which oauth providers are supported

Janpot
Janpot reviewed Jul 30, 2024
View reviewed changes
docs/data/toolpad/core/components/sign-in-page/sign-in-page.md Outdated
The component is composable with any authentication library you might want to use. The following is a functional `SignInPage` with [auth.js](https://authjs.dev/) using GitHub, Next.js App router and server actions.

:::warning
The following demo does not initiate an actual GitHub authentication flow, since doing that from within an `iframe` is not permitted. Run the [Next.js app directory](https://github.com/mui/mui-toolpad/tree/master/examples/core-auth-nextjs/) example to test this functionality.
Copy link
Member

@Janpot Janpot Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the best form. Is this demo really necessary? Perhaps we can remove the demo and create an example instead (not as part of this PR)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added an image instead, remove the extended instructions and linked to the example that is created as part of this PR

Janpot
Janpot reviewed Jul 30, 2024
View reviewed changes
docs/data/toolpad/core/components/account/account.md

When signed out, the component renders as an inline sign in button within the dashboard layout. If a `session` object is present, the component is rendered as a dropdown containing the user's account details as well as an option to sign out.

{{"demo": "AccountDemo.js", "bg": "gradient" }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow up
The demo has some layout shift when signing in

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 31, 2024
Janpot
Janpot reviewed Jul 31, 2024
View reviewed changes
docs/data/toolpad/core/components/page-container/PageContent.js Outdated
@@ -2,7 +2,7 @@ import * as React from 'react';
import Card from '@mui/material/Card';
import CardContent from '@mui/material/CardContent';
import Typography from '@mui/material/Typography';
import Grid from '@mui/material/Unstable_Grid2';
import Grid from '@mui/material/Grid2';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 We may want to keep backwards compatibility with v5.

@mnajdova Will the /Unstable_Grid2 path be maintained during v6?

@bharatkashyap
Copy link
Member Author

Also one more thing: the forgot password links in the demos are kind of weird as they link to the main MUI home page, can we do something about that?

You're right; I've removed them for now - we can add them back once we have those components added?

Janpot
Janpot approved these changes Aug 2, 2024
View reviewed changes
Copy link
Member

@Janpot Janpot left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's merge. We can iterate in follow-up

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 2, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 2, 2024
@bharatkashyap bharatkashyap merged commit f38b7c1 into mui:master Aug 2, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apedroferreira apedroferreira apedroferreira approved these changes

@Janpot Janpot Janpot approved these changes

Assignees
No one assigned
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[core] Add authentication components for @toolpad/core
3 participants
@bharatkashyap @Janpot @apedroferreira